Data structure #89

pantierra · 2017-11-29T00:15:50Z

This is a new pull request for the revised data structure #30 with a lot of wonderful work by @AltNico.
Please help with your review. Thanks! 😄

grote · 2017-11-29T14:12:43Z

@xamanu thanks for this! Do you know why the CI didn't run for this MR? Do you mind rebasing this on master, so we can get the benefit of the Accra integration test to see if it still passes with these changes?

pantierra · 2017-11-29T14:14:02Z

Will rebase and check.

grote

I gave this a first quick read and it already looks quite good. Thanks for the good work!

grote · 2017-11-29T14:28:32Z

osm2gtfs/core/osm_connector.py

+                line.add_itinerary(itinerary)
+            except ValueError:
+                print('Itinerary ID does not match line ID. Please fix in OSM.')
+                print(line.osm_url)


These kind of errors should be printed to stderr like elsewhere.

grote · 2017-11-29T14:31:57Z

osm2gtfs/core/osm_connector.py

-
-        Returns a initiated RouteVariant object from raw data
+
+        colour = "FFFFFF"


I think we should be in line with transitfeed's way of writing color here (and below).

British... American... now it's time to chose 😄 I also tend to use the American over the British as in most technical environments it is the quasi standard. I would go for color, what do you think, @AltNico?

grote · 2017-11-29T14:33:27Z

osm2gtfs/core/osm_connector.py

+
+        colour = "FFFFFF"
+        if "colour" in route_master.tags:
+            colour = OsmConnector.get_hex_code_for_color(route_master.tags['colour'])


Do the tags really use colour instead of color? Is this defined somewhere?

Yes, in OpenStreetMap usually British English is used: https://wiki.openstreetmap.org/wiki/Key:colour

What about supporting both? 😄

grote · 2017-11-29T14:35:28Z

osm2gtfs/core/osm_connector.py

+                        self.stops["relation/" + str(relation.id)
+                                   ] = self._build_stop_area(relation)
+                    except RuntimeError:
+                        print('Cannot add stop area', relation.id)


We should not catch RuntimeErrors. Please use a different exception for this. You can even make your own.

grote · 2017-11-29T14:37:00Z

osm2gtfs/core/osm_connector.py

@@ -598,7 +646,7 @@ def _find_best_name_for_unnamed_stop(self, stop):
        winner_distance = sys.maxint
        for candidate in candidates:
            if isinstance(candidate, overpy.Way):
-                lat, lon = Stop.get_center_of_nodes(
+                lat, lon = OsmConnector.get_center_of_nodes(


We do you need to prepend method calls with OsmConnector when you are in OsmConnector?

grote · 2017-11-29T14:39:34Z

osm2gtfs/core/stops.py

+                      str(osm_type) + "/" + str(osm_id))
+
+
+class StopArea(object):


This doesn't need to be annotated with @attr.s?

grote · 2017-11-29T14:40:24Z

osm2gtfs/core/stops.py

+class Stop(object):
+
+    osm_id = attr.ib()
+    osm_type = attr.ib()


Why do we need an osm_type? What values can this have?

It can be either node or way. Depending on the type different behaviours might be necessary. E.g for a way you may want to find the center. And we concretely need it for constructing the url to the object here.

Another reason is also related to #94: osm_id is used for different id spaces, depending on osm_type. Both together can identify an object in OSM, not only osm_id. For the id of the GTFS it is recommended to combine those two in any way.

grote · 2017-11-29T14:42:06Z

osm2gtfs/creators/incofer/trips_creator_incofer.py

@@ -78,6 +79,7 @@ def _get_itinerary_operation(self, itinerary,
                    operations.append("sunday")
        return operations

+    # pylint: disable=arguments-differ
    def _create_service_period(self, schedule, operation):


Why do the arguments differ here?

grote · 2017-11-29T14:42:45Z

osm2gtfs/creators/routes_creator.py

+        route_url  # From Line
+        route_color # From Line
+        route_text_color  # From Line
+        """


What do these comments mean? What do they relate to?

They were just to not forget important values 😄 Can be taken out.

pantierra · 2017-11-29T19:30:02Z

Thanks a lot for the review, @grote! I just fixed the noted issues.
But I have no idea why CI is not running here.

Please note: When testing, make sure you clean all your cached data.

grote · 2017-11-29T19:36:15Z

Please check your project settings for Integration & Services and make sure Travis CI is listed there and enabled.

grote · 2017-11-29T19:36:57Z

osm2gtfs/core/osm_connector.py


        # Cache data
-        Cache.write_data('stops-' + self.selector, self.stops)
+        Cache.write_data('stops-' + self.selector, self.routes)


routes are written to stops?

grote · 2017-11-29T19:38:57Z

osm2gtfs/core/osm_connector.py

+                    sys.stderr.write("http://osm.org/node/" +
+                                     str(member.ref) + "\n")
+        if len(stop_members) < 1:
+            raise RuntimeError('Cannot build stop area with no members')


Looks like you are still raising a RuntimeError here.

pantierra · 2017-11-29T20:14:10Z

Thanks, now CI pipeline works. Still a bit to fix, but we are close to green 😄 Will comment then.

pantierra · 2017-11-29T20:37:25Z

It seems like the last error raised here by pylint through the Travis CI are part of attr.s and can not be fixed: python-attrs/attrs#215

Author: Felix Delattre <github@xama.nu>

Fixes #30.

pantierra · 2017-12-07T23:19:29Z

Closed in favour of this rebased version: #96

grote requested changes Nov 29, 2017

View reviewed changes

This was referenced Nov 29, 2017

Add config file for Esteli, Nicaragua network #92

Closed

Add config file for Managua, Nicaragua network #88

Closed

pantierra requested review from AltNico, jamescr and nlehuby November 30, 2017 00:11

Felix Delattre and others added 4 commits December 5, 2017 02:34

Implement generic data structure

b88a802

Author: Felix Delattre <github@xama.nu>

Finish work on data structure

83a50cb

Fixes #30.

Adapt existing creators to new data structure

3d81c1c

Minor fixes and clean-ups

b4c6eef

AltNico approved these changes Dec 5, 2017

View reviewed changes

This was referenced Dec 6, 2017

Introduce ISO 3166-2 country codes for creator directories #83

Merged

GTFS stop_id and mapping to OpenStreetMap #94

Closed

WIP: Data structure 2.0 #96

Closed

pantierra closed this Dec 7, 2017

pantierra deleted the data-structure branch December 16, 2017 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data structure #89

Data structure #89

pantierra commented Nov 29, 2017

grote commented Nov 29, 2017

pantierra commented Nov 29, 2017

grote left a comment

grote Nov 29, 2017

grote Nov 29, 2017

pantierra Nov 29, 2017

grote Nov 29, 2017

pantierra Nov 29, 2017

ialokim Nov 29, 2017

grote Nov 29, 2017

grote Nov 29, 2017

grote Nov 29, 2017

grote Nov 29, 2017

pantierra Nov 29, 2017 •

edited

Loading

pantierra Dec 7, 2017

grote Nov 29, 2017

grote Nov 29, 2017

pantierra Nov 29, 2017

pantierra commented Nov 29, 2017

grote commented Nov 29, 2017

grote Nov 29, 2017

grote Nov 29, 2017

pantierra commented Nov 29, 2017 •

edited

Loading

pantierra commented Nov 29, 2017

pantierra commented Dec 7, 2017


		Returns a initiated RouteVariant object from raw data

		colour = "FFFFFF"

Data structure #89

Data structure #89

Conversation

pantierra commented Nov 29, 2017

grote commented Nov 29, 2017

pantierra commented Nov 29, 2017

grote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pantierra Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pantierra commented Nov 29, 2017

grote commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pantierra commented Nov 29, 2017 • edited Loading

pantierra commented Nov 29, 2017

pantierra commented Dec 7, 2017

pantierra Nov 29, 2017 •

edited

Loading

pantierra commented Nov 29, 2017 •

edited

Loading